Add Doltgres DDL constraints to data-model-changes skill#2943
Add Doltgres DDL constraints to data-model-changes skill#2943nick-inkeep wants to merge 3 commits intomainfrom
Conversation
Document every known Doltgres DDL limitation that affects manage schema migrations, with actionable workarounds and a migration review checklist. Every claim was E2E verified: real Drizzle schemas → drizzle-kit 0.31.8 generate → SQL executed against real Doltgres 0.55.5 Docker container. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Adds Doltgres DDL compatibility constraints to the Key changes
Summary ｜ 1 file ｜ 3 commits ｜ base: Doltgres DDL prevention table
Includes a Migration review gate with grep checklist
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thorough, well-evidenced documentation addition. Two actionable issues: broken markdown rendering from nested backticks in the constraint table, and a version-pinning claim that's only partially true. The rest reads cleanly and the grep checklists validate against existing migrations with zero false positives.
Claude Opus ｜ 𝕏
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(3) Total Issues | Risk: Low
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
SKILL.md:29Misleading version pinning claim —docker-compose.dbs.yml(local dev) uses:latest, not the pinned 0.55.5
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
SKILL.md:46Example claims pattern exists in manage-schema but uses runtime-schema column - 🟡 Minor:
SKILL.md:391PR #2929 reference lacks hyperlink
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
SKILL.md:410Forward-looking drizzle-kit v1 beta reference may become stale
🧹 While You're Here (1) 🧹
🧹 1) SKILL.md No cross-reference to manage-database-usage skill
Issue: The data-model-changes skill documents Doltgres DDL constraints but does not reference the existing manage-database-usage skill which covers branch-scoped connections and withRef patterns.
Why: Users who land on this skill for schema changes may not discover the companion skill that explains the critical branch-scoping patterns essential when working with the manage database.
Fix: Add a brief "See also" note after the Doltgres DDL Constraints section: **See also:** The manage-database-usage skill covers branch-scoped connections (withRef, c.get('db')) required when reading/writing to the manage database.
Refs: manage-database-usage skill
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-researched and thoroughly documented skill update. The PR description demonstrates exceptional rigor — every claim was verified through multiple layers of evidence including live Doltgres testing, drizzle-kit source tracing, and end-to-end migration validation. The suggestions above are minor accuracy and usability improvements. The version pinning note deserves attention since developers using pnpm setup-dev will get :latest rather than the documented 0.55.5, but this doesn't invalidate the documented constraints — it just needs clearer context.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
SKILL.md:3 |
Frontmatter description is unusually long (538 chars, 25+ triggers) | Valid design choice for comprehensive discovery; the breadth is intentional per PR description which explains the routing rationale |
SKILL.md:29 |
drizzle-kit version says 0.31.8 but package.json specifies ^0.31.6 | The skill correctly notes "resolved from ^0.31.6" — this is accurate documentation of resolved vs. constraint versions |
| N/A | Docs review findings | File is an internal AI skill file (.agents/skills/), explicitly out of scope for docs review per guidelines |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
5 | 0 | 0 | 1 | 3 | 0 | 1 |
pr-review-consistency |
4 | 0 | 1 | 0 | 1 | 0 | 2 |
pr-review-docs |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
| Total | 10 | 0 | 1 | 1 | 4 | 0 | 4 |
Note: Both devops and consistency reviewers flagged the same Doltgres version inconsistency — findings were deduplicated.
- Fix nested backtick rendering in constraint table (lines 38, 41) - Clarify version pinning: note docker-compose.dbs.yml uses :latest - Replace fictional status column example with real credentialScope from manage-schema.ts - Add hyperlink to PR #2929 precedent - Remove specific beta version number to reduce staleness Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review feedback addressed (498c961)All 7 review threads resolved. Summary:
|
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta review scoped to the changes since the prior automated review (commit bf98fe4d6 → 498c961ed).
✅ Prior Feedback Addressed
All 4 issues from the prior reviews have been properly resolved:
| Prior Finding | Status | Verification |
|---|---|---|
| 🟠 Version pinning claim misleading | ✅ Fixed | Line 29 now notes that docker-compose.dbs.yml and docker-compose.isolated.yml use :latest |
🟡 Example used wrong column (status) |
✅ Fixed | Line 50 now uses credentialScope — verified against manage-schema.ts:429 |
| 🟡 PR #2929 lacked hyperlink | ✅ Fixed | Line 391 now has proper markdown link |
| 🟡 Nested backticks broke markdown | ✅ Fixed | Lines 38, 41 use double-backtick escaping |
🕐 Pending Recommendations (1)
- 🧹
SKILL.mdNo cross-reference to manage-database-usage skill (optional enhancement from prior review — pre-existing gap, not blocking)
✅ APPROVE
Summary: The delta cleanly addresses all prior feedback. The credentialScope example was verified against the actual schema, the version note accurately reflects the compose file configurations, and the PR #2929 reference links to a real merged PR. This is a well-researched skill update with thorough evidence backing every claim. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta-only review — no sub-reviewers dispatched since all changes are documentation fixes addressing prior feedback.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta review scoped to the changes since the prior automated review (commit 498c961ed → 45362a5ec).
✅ Delta Analysis
Commit: 45362a5ec — "Tighten intro paragraph — remove redundant sentence"
The single change condenses two sentences into one:
- Before: "Doltgres does not support the full PostgreSQL DDL surface. drizzle-kit generates standard PostgreSQL SQL, which means certain Drizzle schema patterns produce migrations that fail on Doltgres."
- After: "drizzle-kit generates standard PostgreSQL SQL that may be incompatible with Doltgres."
This is a clean editorial improvement — the redundant first sentence ("Doltgres does not support the full PostgreSQL DDL surface") is subsumed by the second, and "may be incompatible" is appropriately softer than "fail" since the constraints depend on which DDL patterns are used.
🕐 Pending Recommendations (1)
- 🧹
SKILL.mdNo cross-reference to manage-database-usage skill — author declined per write-skill principles (skills should be standalone; both are independently discoverable via triggers)
✅ APPROVE
Summary: The delta is a purely editorial improvement that makes the intro more concise without changing meaning. All substantive feedback from prior reviews was addressed. This well-researched skill update with thorough evidence backing every Doltgres constraint is ready to ship. 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta-only review — no sub-reviewers dispatched. The change is a trivial editorial refinement that does not warrant re-review of the entire file.
1 participant
Summary
Add Doltgres DDL compatibility constraints to the
data-model-changesskill — the 8 Drizzle schema patterns that produce migrations incompatible with Doltgres, the 2 drizzle-kit outputs that require manual editing, and a grep-based review checklist.One file changed:
.agents/skills/data-model-changes/SKILL.md(+79, -8)What's in the skill update
Section A: Prevention table (8 constraints)
pgEnum()alter_type.go:34—ALTER TYPE is not yet supportedAlterTypeAddValueConvertorALTER TYPE ADD VALUE→ hard errorpgSchema()alter_table.go:96—ALTER TABLE SET SCHEMA is not yet supportedPgAlterTableSetSchemaConvertorserial()/pgSequence()alter_sequence.go:105— generic unsupported handlerAlterPgSequenceConvertorindex().where()create_index.go:33—WHERE is not yet supportedCreatePgIndexConvertor(pass-through)index().concurrently()create_index.go:27—concurrent index creation is not yet supportedindex().using('gin'/'gist'/'hash')create_index.go:30—index method %s is not yet supportedindex().on(sql\...`)`index_elem.go:32—expression index attribute is not yet supportedcol.desc()index_elem.go:57—NULLS LAST for indexes is not yet supported.desc()silently emitsDESC NULLS LASTSection B: Migration review gate (Step 7 + Step 3)
DROP TABLE ... CASCADEsqlgenerator.ts:1543— unconditional CASCADEdrop_table.go:46—CASCADE is not yet supportedALTER COLUMN ... USINGsqlgenerator.ts:1943— branches 3-4 add USINGalter_table.go:379—ALTER TABLE with USING is not supported yetAlso added
Verification method
Every constraint tested end-to-end: Drizzle TypeScript schema →
npx drizzle-kit generate(v0.31.8) → inspect SQL → execute against real Doltgres 0.55.5 Docker container. Both workarounds (multi-step type change, pgEnum→varchar escape) also E2E tested with data integrity checks.Test plan
sqlgenerator.ts)server/ast/)manage-schema.ts🤖 Generated with Claude Code